C# Fixes wasi http header bug and adds a test for it#1215
C# Fixes wasi http header bug and adds a test for it#1215silesmo wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
jsturtevant
left a comment
jsturtevant
left a comment
There was a problem hiding this comment.
Is the fix here around the alignment? How was the issue presenting?
| " | ||
| void* {address}; | ||
| if (({size} * {list}.Count) < 1024) {{ | ||
| var {ret_area} = stackalloc {element_type}[({array_size}*{list}.Count)+1]; |
There was a problem hiding this comment.
looks like this moved the logic to add the additional 1 to the dotnet_aligned_array function, we should update
wit-bindgen/crates/csharp/src/function.rs
Line 1294 in 6541e44
| void* {address}; | ||
| if (({size} * {list}.Count) < 1024) {{ | ||
| var {ret_area} = stackalloc {element_type}[({array_size}*{list}.Count)+1]; | ||
| {address} = (void*)(((int){ret_area}) + ({align} - 1) & -{align}); |
There was a problem hiding this comment.
I believe this line is still required or in some cases this won't align properly since stackalloc won't align on the correct memory boundary (dotnet/csharplang#1799). Unless something else is subtly addressing this?
There was a problem hiding this comment.
Refactor void* AlignStackPtr(void* stackAddress, uint alignment) helper and use it everywhere.
| ); | ||
| } | ||
| results.push(format!("(nint){ptr}")); | ||
| results.push(format!("(int){ptr}")); |
There was a problem hiding this comment.
Because it generates
BitConverter.TryWriteBytes(new Span<byte>((void*)(basePtr + 8), 4), (nint)listPtr);And that uses TryWriteBytes(Span<Byte>, UInt64) which is wrong. There is no TryWriteBytes overload for nint.
And then it returns false but we ignore it.
Let's stop using TryWriteBytes.
Instead it could be just
new Span<nint>((void*)(basePtr + 8), 1)[0] = listPtr;
|
|
||
| string-roundtrip: func(a: string) -> string; | ||
|
|
||
| wasi-http-headers-roundtrip: func(a: list<tuple<string, list<u8>>>) -> list<tuple<string, list<u8>>>; |
There was a problem hiding this comment.
will need to add this to https://github.com/bytecodealliance/wit-bindgen/tree/main/tests/runtime-new/lists since this test has moved to the new framework #1221
|
I ran into this issue as well. Are there any plans to land this fix or similar? Thanks 🙏 |
4 participants
Fixes a bug where headers returned from a C# component wouldn't be properly set. Adds a test for it and some cleanup.